fix: defer framework adapter creation until YAML is loaded (fixes #1877)#1881
fix: defer framework adapter creation until YAML is loaded (fixes #1877)#1881praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
|
@coderabbitai review |
|
/review |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
✅ Action performedReview finished.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAgentsGenerator defers framework adapter creation from initialization to the crew generation phase, allowing YAML framework configuration to be read before instantiating the adapter. The default framework fallback changes from crewai to praisonai when neither the CLI nor YAML explicitly specify a framework. ChangesFramework initialization lazy-loading and default fallback
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes a crash on
Confidence Score: 4/5The two-line production change is safe; the test suite has a broken fixture that will fail CI. The core fix in src/praisonai/tests/unit/test_agents_generator_async.py — the two older test functions need their Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI
participant AgentsGenerator
participant _prepare_for_run
participant FrameworkAdapter
CLI->>AgentsGenerator: "__init__(framework="")"
Note over AgentsGenerator: self.framework_adapter = None (deferred)
CLI->>AgentsGenerator: generate_crew_and_kickoff()
AgentsGenerator->>AgentsGenerator: yaml.safe_load(agent_file)
AgentsGenerator->>_prepare_for_run: _prepare_for_run(config)
_prepare_for_run->>_prepare_for_run: "framework = self.framework or config.get('framework', 'praisonai')"
_prepare_for_run->>FrameworkAdapter: _get_framework_adapter(framework).resolve()
FrameworkAdapter-->>_prepare_for_run: adapter
_prepare_for_run->>_prepare_for_run: assert_framework_available(adapter.name)
_prepare_for_run->>_prepare_for_run: init_observability(adapter.name)
_prepare_for_run->>FrameworkAdapter: "adapter.setup(framework_tag=adapter.name)"
_prepare_for_run->>AgentsGenerator: "self.framework_adapter = adapter"
_prepare_for_run-->>AgentsGenerator: prep dict
AgentsGenerator->>FrameworkAdapter: adapter.run(...) or adapter.arun(...)
|
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/praisonai/praisonai/agents_generator.py (2)
743-743:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Async path still uses 'crewai' default instead of 'praisonai'.
The synchronous path (line 597) correctly changed the default fallback to
'praisonai', but this async path still uses'crewai'. This creates inconsistent behavior:
generate_crew_and_kickoff()→ defaults to praisonaiagenerate_crew_and_kickoff()→ defaults to crewaiThis violates the PR objective to change the default framework to
'praisonai'and causes sync/async divergence.🔧 Proposed fix
- framework = self.framework or config.get('framework', 'crewai') + framework = self.framework or config.get('framework', 'praisonai')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/praisonai/praisonai/agents_generator.py` at line 743, The async path still falls back to 'crewai'—update the fallback default to 'praisonai' to match the sync path; locate the assignment to framework in agenerate_crew_and_kickoff (the line "framework = self.framework or config.get('framework', 'crewai')") and change the config.get default to 'praisonai' so both synchronous (generate_crew_and_kickoff) and asynchronous code use the same default framework.
776-796:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCritical: Async path missing key initialization steps present in sync path.
The async path has incomplete framework adapter initialization compared to the sync path. Missing components:
- Line 778: No
.resolve()call on the adapter (sync path line 601 calls it)- No observability initialization: Sync path calls
init_observability(adapter.name)(line 609)- No adapter setup hook: Sync path calls
adapter.setup(framework_tag=adapter.name)(line 612)- Line 782: Uses
frameworkstring instead ofadapter.namefor validation (sync path line 605 usesadapter.nameafter resolution)The
.resolve()call is particularly important as it handles framework variant selection (e.g., AutoGen version selection), and without it, the adapter may not be properly configured.🔧 Proposed fix to align async with sync initialization
# Update framework adapter if framework changed if framework != self.framework: self.framework = framework - self.framework_adapter = self._get_framework_adapter(framework) + initial_adapter = self._get_framework_adapter(framework) + self.framework_adapter = initial_adapter.resolve() # Validate framework availability from .framework_adapters.validators import assert_framework_available - assert_framework_available(framework) + assert_framework_available(self.framework_adapter.name) + + # Initialize observability hooks + from .observability.hooks import init_observability + init_observability(self.framework_adapter.name) + + # Run adapter setup hooks + self.framework_adapter.setup(framework_tag=self.framework_adapter.name) # Validate cli_backend compatibility - self._validate_cli_backend_compatibility(config, framework) + self._validate_cli_backend_compatibility(config, self.framework_adapter.name) - self.logger.info(f"Using framework: {framework}") + self.logger.info(f"Using framework: {self.framework_adapter.name}")Note: You may also want to remove or refactor the AutoGen version selection logic (lines 746-764) if
.resolve()now handles it internally, to avoid duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/praisonai/praisonai/agents_generator.py` around lines 776 - 796, The async branch of run/generate is missing adapter initialization steps present in the sync branch: after obtaining the adapter with _get_framework_adapter(framework) call adapter.resolve() to select the correct variant, then call init_observability(adapter.name) and adapter.setup(framework_tag=adapter.name) before running _validate_cli_backend_compatibility; also change the assert_framework_available(framework) / _validate_cli_backend_compatibility calls to use adapter.name (not the raw framework string) so validation targets the resolved adapter, then proceed to await self.framework_adapter.arun(...) as before.
🧹 Nitpick comments (1)
src/praisonai/praisonai/agents_generator.py (1)
746-764: Reduce AutoGen version selection duplication (sync usesadapter.resolve(), async hardcodes logic)Sync path calls
initial_adapter.resolve()(viaadapter = initial_adapter.resolve()), andAutoGenAdapter.resolve()performs AutoGen v0.2 vs v0.4 selection based onAUTOGEN_VERSION. Async path instead applies its own AutoGen selection logic and only calls_get_framework_adapter(framework)after rewritingframework(noresolve()call).This duplicates the same decision-making and can also create inconsistent behavior because async considers
config.get('autogen_version', ...), whileAutoGenAdapter.resolve()reads onlyos.environ["AUTOGEN_VERSION"].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/praisonai/praisonai/agents_generator.py` around lines 746 - 764, Replace the manual AutoGen version selection block with the same resolve-based flow used by the sync path: obtain the initial adapter via _get_framework_adapter("autogen") and call its resolve() (i.e., use AutoGenAdapter.resolve()) to determine the concrete adapter/framework instead of re-implementing autogen_version logic; remove the ad-hoc uses of autogen_v4_adapter, autogen_v2_adapter, autogen_version, and use_v4 and set framework based on the resolved adapter so async and sync share the same decision logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/praisonai/praisonai/agents_generator.py`:
- Line 743: The async path still falls back to 'crewai'—update the fallback
default to 'praisonai' to match the sync path; locate the assignment to
framework in agenerate_crew_and_kickoff (the line "framework = self.framework or
config.get('framework', 'crewai')") and change the config.get default to
'praisonai' so both synchronous (generate_crew_and_kickoff) and asynchronous
code use the same default framework.
- Around line 776-796: The async branch of run/generate is missing adapter
initialization steps present in the sync branch: after obtaining the adapter
with _get_framework_adapter(framework) call adapter.resolve() to select the
correct variant, then call init_observability(adapter.name) and
adapter.setup(framework_tag=adapter.name) before running
_validate_cli_backend_compatibility; also change the
assert_framework_available(framework) / _validate_cli_backend_compatibility
calls to use adapter.name (not the raw framework string) so validation targets
the resolved adapter, then proceed to await self.framework_adapter.arun(...) as
before.
---
Nitpick comments:
In `@src/praisonai/praisonai/agents_generator.py`:
- Around line 746-764: Replace the manual AutoGen version selection block with
the same resolve-based flow used by the sync path: obtain the initial adapter
via _get_framework_adapter("autogen") and call its resolve() (i.e., use
AutoGenAdapter.resolve()) to determine the concrete adapter/framework instead of
re-implementing autogen_version logic; remove the ad-hoc uses of
autogen_v4_adapter, autogen_v2_adapter, autogen_version, and use_v4 and set
framework based on the resolved adapter so async and sync share the same
decision logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e7ee4cd-242a-4193-924d-5889257b201a
📒 Files selected for processing (1)
src/praisonai/praisonai/agents_generator.py
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Architecture Review & Fix Implementation ✅I've conducted a comprehensive architecture review and successfully implemented critical fixes to align the async path with the sync path. Phase 1: Architecture Review ✅
Phase 2: Critical Issues Fixed ✅🔧 Fixed in commit
Phase 3: Final Verdict ✅APPROVED - All critical issues have been resolved:
The PR now correctly implements the deferred framework adapter initialization while maintaining feature parity between sync and async execution paths. The "better defaults" improvement (praisonai vs crewai) is consistently applied across both paths. Architecture compliance: All changes align with AGENTS.md protocol-driven design principles. Files Modified:
What Was Skipped:
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Merge review — AGENTS.md gap (CI green, merge CLEAN) CI ✅ | wrapper/YAML fix (#1877) Fix list:
Tests mandatory before merge. |
|
Claude finished @MervinPraison's task —— View job Merge Review & Test Implementation ✅Added mandatory tests and documentation before merge approval. Todo List:
Summary of Changes✅ Tests Added (commit
✅ Documentation Added:
✅ Tests Pass: Both new tests verify the fix works correctly and async/sync paths have proper parity. Architecture Review ConclusionThe PR now meets all AGENTS.md requirements:
READY FOR MERGE - All mandatory requirements satisfied. |
|
Status worsened — merge conflicts on This branch is now CONFLICTING / DIRTY. Please:
Do not merge — fix and push only. |
|
Claude finished @MervinPraison's task —— View job Rebase and Conflict Resolution ✅Successfully resolved merge conflicts and rebased onto latest main. Todo List:
Summary of Resolution✅ Successfully rebased ✅ Conflicts resolved in:
✅ Key fixes preserved after rebase:
✅ Branch pushed with force-with-lease to ✅ CI triggered - Rebase completed successfully, ready for merge review The branch now incorporates all changes from the latest main while preserving the critical fixes for issue #1877. Both reviewer concerns (framework default consistency and async path initialization) have been maintained through the rebase process. |
praisonai-triage-agent
Bot
force-pushed
the
claude/issue-1877-20260610-0601
branch
from
c0acbe8 to
e002ebf
Compare
|
@claude This PR has merge conflicts with
After rebase, re-run CI and confirm merge is clean. |
|
Claude finished @MervinPraison's task —— View job Rebase and Conflict Resolution ✅Successfully resolved merge conflicts and rebased onto latest main. Todo List:
Summary of Resolution✅ Successfully rebased ✅ Conflicts resolved in:
✅ Key fixes preserved after rebase:
✅ Branch pushed with force-with-lease to ✅ CI triggered - Rebase completed successfully, ready for merge review The branch now incorporates all changes from the latest main while preserving the critical fixes for issue #1877. Both reviewer concerns (framework default consistency and async path initialization) have been maintained through the rebase process. |
praisonai-triage-agent
Bot
force-pushed
the
claude/issue-1877-20260610-0601
branch
from
e002ebf to
188297d
Compare
|
@claude Rebase required (PR #1881 — fixes #1877) Merge state: Hot file: Action: git fetch origin main && git rebase origin/main
# Keep defer-YAML-load behaviour; integrate with current agents_generator async/sync paths from main
git push --force-with-leaseCI on last green run: |
|
Claude finished @MervinPraison's task —— View job Rebase and Conflict Resolution ✅Successfully resolved merge conflicts and rebased onto latest main. Todo List:
Summary of Resolution✅ Successfully rebased ✅ No conflicts encountered - The rebase completed cleanly without any merge conflicts. ✅ Key fixes preserved after rebase:
✅ Branch pushed with force-with-lease to ✅ CI triggered - Rebase completed successfully, ready for merge review The branch now incorporates all changes from the latest main while preserving the critical fixes for issue #1877. The deferred YAML framework loading behavior has been maintained through the rebase process. |
- Fixes issue where AgentsGenerator crashes with empty framework before YAML is read - Defers framework_adapter creation from __init__ to generate_crew_and_kickoff() - Changes default YAML framework from 'crewai' to 'praisonai' for better onboarding - Enables 'praisonai agents.yaml' to work without --framework flag Fixes #1877 Co-authored-by: praisonai-triage-agent[bot] <praisonai-triage-agent[bot]@users.noreply.github.com>
… parity - Add test verifying YAML files without framework key default to praisonai (fixes #1877) - Add test ensuring async path has same adapter.resolve() + init_observability() pattern as sync - Add FRAMEWORK_DEFAULTS_CHANGE.md documenting breaking change migration 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
praisonai-triage-agent
Bot
force-pushed
the
claude/issue-1877-20260610-0601
branch
from
188297d to
85a8912
Compare
1 participant
Fixes #1877
Summary
Root Cause
The framework adapter was created in AgentsGenerator.init() before the YAML file was parsed. When running praisonai agents.yaml, the CLI passes an empty framework string, causing immediate failure.
Changes
Impact
Testing
The fix follows the protocol-driven design from AGENTS.md and maintains all existing functionality while resolving the initialization order issue.
Generated with Claude Code
Summary by CodeRabbit